Don't send unrecognized featureSets to deadletter in IngestionJob #845
Conversation
| if (featureSetSpecs == null) { | ||
| log.warn( | ||
| String.format( | ||
| "FeatureRow contains invalid feature set id %s. Please check that the feature rows are being published to the correct topic on the feature stream.", |
There was a problem hiding this comment.
We are now calling it a feature set reference.
|
/kind bug |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pyalex, woop The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
|
/test test-end-to-end-redis-cluster |
|
This feels like a somewhat arbitrary technical fix to an operational problem. Semantically I would consider unrecognized feature set a "dead letter" just as much as the remaining issues that route there. For us, this could be a (SQL) replication delay from the registry when a producer already started publishing data, a case where we may want to be able to reprocess. On the other hand Feast does need a means of protecting itself from misuse, e.g. data being erroneously sent without any intended registration (deployment in wrong environment, etc.). Perhaps separate error queues. I don't strongly object to this going in, especially as it's currently with dead letters coupled to BigQuery, but it does bring to mind that this is an area of debt for the backlog IMO: the BigQuery coupling, and new design considerations that could accompany tackling that. |
Agreed on that. The whole deadletter and logging area needs TLC. So yes, this is a bit of a workaround until its properly solved. |
What this PR does / why we need it:
Currently we overpopulating deadletter sink (in bq) by sending all FeatureRows that coming in with unknown FeatureSetReference. That could create billions of rows within hours and create long queue in IngestionJob, which disrupt normal processing (OOM, eg). We should just write smth to log instead.
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: